Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Save changes before closing interaction. #12166

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Save changes before closing interaction. #12166

merged 1 commit into from
Feb 23, 2023

Conversation

emilhammarstedtst
Copy link
Contributor

@emilhammarstedtst emilhammarstedtst commented Feb 9, 2023 •
edited
Loading

Screenshot from 2023-02-09 13-44-36

What it does

  • Question to save all changes, cancel or discard all changes before closing
  • Display what is not yet saved.
  • Reorder options and reword the question and options to be clear of the outcome of each action
  • Implemented using ConfirmDialogSave extending ConfirmDialog

How to test

Electron build of theia and make one or more editors dirty then click the "X" to close the theia window. the popup should appear with a question to save.

Review checklist

Reminder for reviewers

Contributed by STMicroelectronics

vince-fugnitto
vince-fugnitto reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilhammarstedtst thank you for the contribution 👍

I have some general comments about the dialog, mainly to align with the one present in VS Code and re-use some already existing localizations.

I believe that it is incorrect to default to the "Quit Without Saving" button which is selected by default. Pressing Enter should probably Save All instead similarly to VS Code.

Please also be sure to properly fill out the pull-request description with "How To Test", for example I don't believe these changes apply to the browser as you have them today?

packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
@emilhammarstedtst emilhammarstedtst force-pushed the pr/save-changes-on-quit branch 2 times, most recently from 1361177 to db1b421 Compare February 9, 2023 14:55
@emilhammarstedtst
Copy link
Contributor Author

@emilhammarstedtst thank you for the contribution +1

I have some general comments about the dialog, mainly to align with the one present in VS Code and re-use some already existing localizations.

I believe that it is incorrect to default to the "Quit Without Saving" button which is selected by default. Pressing Enter should probably Save All instead similarly to VS Code.

Please also be sure to properly fill out the pull-request description with "How To Test", for example I don't believe these changes apply to the browser as you have them today?

Amended the commit with your suggestions and cleared up the "undefined"-leftover.
However the title now potentially has the the text " following 0 files?", so i created a fallback case for this.

I dont believe the browser is in question as it is bound to use the onbeforeunload method?

@emilhammarstedtst emilhammarstedtst force-pushed the pr/save-changes-on-quit branch 2 times, most recently from dce369c to e7ea5ea Compare February 10, 2023 16:08
msujew
msujew approved these changes Feb 17, 2023
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 I have 2 minor comments before merging though.

packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
@emilhammarstedtst
Save changes before closing interaction.
4aec3d4 
- Question to save all changes, cancel or discard all changes before closing
- Display what is not yet saved.
- Reorder options and reword the question and options to be clear of the outcome of each action
- Implemented using ConfirmDialogSave extending ConfirmDialog

Contributed by STMicroelectronics

Signed-off-by: Emil Hammarstedt <emil.hammarstedt@st.com>
@emilhammarstedtst emilhammarstedtst mentioned this pull request Feb 21, 2023
Closed
@vince-fugnitto
Copy link
Member

@msujew do we want to merge for today's release?

@msujew msujew merged commit 7fd510e into eclipse-theia:master Feb 23, 2023
@vince-fugnitto vince-fugnitto added this to the 1.35.0 milestone Feb 23, 2023
@vince-fugnitto vince-fugnitto added the core issues related to the core of the application label Feb 23, 2023
@vince-fugnitto vince-fugnitto mentioned this pull request Mar 1, 2023
Closed
This was referenced Mar 22, 2024
Open
Open
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@msujew msujew msujew approved these changes

Assignees
No one assigned
Labels
core issues related to the core of the application
Projects
None yet
Milestone
1.35.0
Development

Successfully merging this pull request may close these issues.
3 participants
@emilhammarstedtst @vince-fugnitto @msujew